Add initial implementation of std::dump
#1
Conversation
foonathan
requested review from
bretbrownjr,
camio,
dietmarkuehl,
neatudarius and
steve-downey
as code owners
foonathan
force-pushed
the
impl
branch
2 times, most recently
from
b189fdf to
c442ff7
Compare
include/beman/dump/dump.hpp
Outdated
| namespace beman::dump { | ||
| namespace detail { | ||
| template <std::size_t N> | ||
| constexpr auto format_string = []{ |
There was a problem hiding this comment.
A documentation comment for this variable template would be nice.
| target_link_libraries(beman.dump.tests | ||
| PRIVATE beman::dump GTest::gtest GTest::gtest_main) | ||
|
|
||
| gtest_add_tests(beman.dump.tests "" AUTO) |
There was a problem hiding this comment.
I think we need some kind of test that verifies that the dump() call is implemented correctly. One way this could be done is to:
- Write a short CMake script that runs a program that calls
dump(), pipes it to a file, and verifies its output - Register the CMake execution as a test using
add_test
There was a problem hiding this comment.
It is almost trivial to redirect std::cout though, I don't see the need to write to a file, but we can discuss this later.
| Full runable examples can be found in `examples/` (e.g., [./examples/identity_as_default_projection.cpp.cpp](./examples/identity_as_default_projection.cpp.cpp)). | ||
|
|
||
| ## Building beman.example | ||
| ## Building beman.dump |
There was a problem hiding this comment.
There should be a ## Usage section demonstrating dump() calls.
|
I was given green light to take over this PR by email. I will try to clear up this PR and get it merged this week. Thanks for your contribution @foonathan ! |
Co-authored-by: David Sankel <[email protected]>
Co-authored-by: David Sankel <[email protected]>
Co-authored-by: David Sankel <[email protected]>
Co-authored-by: David Sankel <[email protected]>
Co-authored-by: David Sankel <[email protected]>
Co-authored-by: David Sankel <[email protected]>
Co-authored-by: David Sankel <[email protected]>
Co-authored-by: David Sankel <[email protected]>
|
To prioritize merging first, I have resolved all trivial suggestions. Summary of TODOs:
|
|
Would suggest prioritizing merging this PR to all reviewers. |
|
FYI, the underlying proposal was rejected; cplusplus/papers#2034 (comment) |
|
PR was merged because this library will be deprecated. No future work in this repo. |
No description provided.